Skip to content

feat: complete HTTP QUERY method support (follow-up to #4436)#4456

Merged
ReneWerner87 merged 2 commits into
mainfrom
feat/http-query-method-followup
Jun 25, 2026
Merged

feat: complete HTTP QUERY method support (follow-up to #4436)#4456
ReneWerner87 merged 2 commits into
mainfrom
feat/http-query-method-followup

Conversation

@ReneWerner87

Copy link
Copy Markdown
Member

Description

Follow-up to #4436 (HTTP QUERY method, RFC 10008), filling gaps found in review and aligning the QUERY cache key with the keygen refactor from #4450 (already on main).

Changes

  • client: add Client.Query() and package-level Query() so the QUERY verb has the same shorthands as every other method. 🔥 feat: Add HTTP QUERY method support (RFC 10008) #4436 only added Request.Query(), leaving the Client API inconsistent.
  • cache: escape the QUERY request body and append it via appendBoundKeySegment, consistent with the per-dimension escaping the keygen already applies to path/query/headers/cookies. Before this the body was appended raw and in the old return-string style, which left a stylistic inconsistency and a latent key-injection asymmetry (not exploitable today, since the body is the terminal key dimension). Pinned with new QUERY golden cases in keygen_test.go, including a delimiter case (a|b:c -> b=a\pb\cc).
  • tests: add direct IsMethodSafe/IsMethodIdempotent assertions for QUERY (none existed), cover QUERY in the CSRF safe-method tests, and add Test_Client_Query.
  • docs: add QUERY to the CORS AllowMethods default, the Register interface, the safe-method lists (idempotency, earlydata, csrf), the cache body-key note, and the client REST reference.

Type of change

  • Bug fix / hardening (cache-key body escaping)
  • New feature (client QUERY shorthands)
  • Documentation update

Checklist

  • Conducted a self-review of the code.
  • Updated the documentation under /docs/.
  • Added/updated unit tests.
  • Ensured new and existing unit tests pass locally (go test ./..., all packages green).
  • Aimed for optimal performance with minimal allocations (escapeKeyDelimiters has a no-alloc fast path for bodies without delimiters).

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

The PR adds QUERY client helpers and route documentation, expands redirect handling for QUERY requests, updates cache-key construction for QUERY bodies, and revises middleware method classifications, docs, and tests to include QUERY behavior.

Changes

QUERY method support

Layer / File(s) Summary
Query API surface
client/client.go, client/client_test.go, docs/client/rest.md, docs/api/app.md, app_test.go, docs/whats_new.md
(*Client).Query and the package-level Query helper are added, the REST and route-chain docs list QUERY, and the app/client tests exercise the new request and route surface.
Redirect handling
client/core.go, client/request_test.go
execFunc treats QUERY as redirect-eligible, and the request test follows a 308 redirect while preserving the body.
QUERY cache keys
middleware/cache/cache.go, middleware/cache/cache_test.go, docs/middleware/cache.md
QUERY cache keys now append an escaped body segment in middleware, and the cache test/docs cover body-sensitive lookups when KeyGenerator ignores the body.
Safe-method coverage
helpers_test.go, docs/middleware/cors.md, docs/middleware/earlydata.md, middleware/earlydata/*, docs/middleware/idempotency.md, middleware/idempotency/*
Method-category helpers and middleware docs/tests add QUERY to the safe/idempotent/default method lists, and the early-data and idempotency tests exercise the new handling.
CSRF validation
middleware/csrf/*, docs/middleware/csrf.md
CSRF middleware removes QUERY from the safe-method branch, and the CSRF test/docs validate token enforcement for QUERY requests.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • gofiber/fiber#3443: Revisits the HTTP method-category rules that the QUERY safe/idempotent changes extend.
  • gofiber/fiber#4224: Changes cache-key generation behavior that the QUERY body-folding update modifies.
  • gofiber/fiber#4436: Introduces QUERY support that this PR extends across cache and CSRF behavior.

Suggested labels

codex

Suggested reviewers

  • gaby
  • sixcolors
  • efectn

Poem

A rabbit hopped where QUERY grew,
With cache and redirects in morning dew.
Safe paws tapped, then CSRF binkied near,
The Allow header twitched its ear.
🐰 Hop! The body came back true.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 23.08% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and accurately summarizes the main change: adding full HTTP QUERY support as a follow-up to #4436.
Description check ✅ Passed The description covers purpose, change summary, type of change, and a checklist, with only some optional template items left blank.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/http-query-method-followup

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@ReneWerner87 ReneWerner87 added this to v3 Jun 25, 2026
@ReneWerner87 ReneWerner87 added this to the v3 milestone Jun 25, 2026
@codecov

codecov Bot commented Jun 25, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.80%. Comparing base (9df17c3) to head (948553f).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4456      +/-   ##
==========================================
+ Coverage   91.70%   91.80%   +0.10%     
==========================================
  Files         134      134              
  Lines       13498    13504       +6     
==========================================
+ Hits        12378    12398      +20     
+ Misses        713      699      -14     
  Partials      407      407              
Flag Coverage Δ
unittests 91.80% <100.00%> (+0.10%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/middleware/cors.md`:
- Line 117: The Default Config example in the CORS docs is missing
fiber.MethodQuery in AllowMethods, causing it to diverge from the documented
table and the actual middleware defaults. Update the default configuration
snippet in the CORS docs to include fiber.MethodQuery in the AllowMethods list,
matching the behavior defined by the cors config/defaults and keeping the
example consistent with the AllowMethods entry.

In `@helpers_test.go`:
- Around line 1831-1832: The local slice named unsafe in helpers_test.go is
shadowing the unsafe import name and triggering the revive import-shadowing lint
failure. Rename the safe and unsafe slices in the relevant test setup to
non-conflicting names such as safeMethods and unsafeMethods, and update the
nearby assertions/usages accordingly so the test logic stays the same while
avoiding the import shadow.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 83824e8f-4f1d-47de-8502-d02ed1837c92

📥 Commits

Reviewing files that changed from the base of the PR and between 9df17c3 and c339022.

📒 Files selected for processing (13)
  • client/client.go
  • client/request_test.go
  • docs/api/app.md
  • docs/client/rest.md
  • docs/middleware/cache.md
  • docs/middleware/cors.md
  • docs/middleware/csrf.md
  • docs/middleware/earlydata.md
  • docs/middleware/idempotency.md
  • helpers_test.go
  • middleware/cache/cache.go
  • middleware/cache/keygen_test.go
  • middleware/csrf/csrf_test.go

Comment thread docs/middleware/cors.md
Comment thread helpers_test.go Outdated
Follow-up filling gaps found in review and aligning the cache key with the
keygen refactor already on main (#4450):

- client: add Client.Query and package-level Query to match every other verb
  shorthand (previously only Request.Query existed)
- cache: escape the QUERY body and append it via appendBoundKeySegment,
  consistent with the per-dimension escaping on main (was raw, old-style);
  pin the exact format with QUERY golden cases in keygen_test.go
- tests: assert IsMethodSafe/IsMethodIdempotent for QUERY; cover QUERY in the
  CSRF safe-method tests; add Test_Client_Query
- docs: add QUERY to CORS AllowMethods default, the Register interface, the
  safe-method lists (idempotency, earlydata, csrf), the cache body-key note,
  and the client REST reference

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@ReneWerner87
ReneWerner87 force-pushed the feat/http-query-method-followup branch from b7a9181 to e5fd66f Compare June 25, 2026 08:37

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
client/client_test.go (1)

1251-1251: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Remove fixed sleep from test setup.

time.Sleep(1 * time.Second) makes the test slower and can still be flaky as a readiness mechanism; prefer relying on startTestServerWithPort readiness (or polling the endpoint with timeout if needed).

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/client_test.go` at line 1251, The test setup in client_test.go uses a
fixed time.Sleep as a readiness wait, which slows the suite and can still be
flaky. Remove the sleep from the test flow around startTestServerWithPort and
rely on the server’s readiness signal instead; if readiness is still uncertain,
replace it with a bounded polling check against the endpoint using a timeout.
Keep the change localized to the test helper or setup path that currently waits
before proceeding.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@client/client_test.go`:
- Line 1251: The test setup in client_test.go uses a fixed time.Sleep as a
readiness wait, which slows the suite and can still be flaky. Remove the sleep
from the test flow around startTestServerWithPort and rely on the server’s
readiness signal instead; if readiness is still uncertain, replace it with a
bounded polling check against the endpoint using a timeout. Keep the change
localized to the test helper or setup path that currently waits before
proceeding.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: e40bdee4-accd-4fb7-bdfb-04a70858f7a1

📥 Commits

Reviewing files that changed from the base of the PR and between b7a9181 and e5fd66f.

📒 Files selected for processing (13)
  • client/client.go
  • client/client_test.go
  • docs/api/app.md
  • docs/client/rest.md
  • docs/middleware/cache.md
  • docs/middleware/cors.md
  • docs/middleware/csrf.md
  • docs/middleware/earlydata.md
  • docs/middleware/idempotency.md
  • helpers_test.go
  • middleware/cache/cache.go
  • middleware/cache/keygen_test.go
  • middleware/csrf/csrf_test.go
✅ Files skipped from review due to trivial changes (7)
  • docs/middleware/earlydata.md
  • docs/api/app.md
  • middleware/csrf/csrf_test.go
  • docs/middleware/cache.md
  • docs/middleware/idempotency.md
  • docs/middleware/csrf.md
  • docs/middleware/cors.md
🚧 Files skipped from review as they are similar to previous changes (5)
  • middleware/cache/cache.go
  • helpers_test.go
  • client/client.go
  • docs/client/rest.md
  • middleware/cache/keygen_test.go

…ase notes

A second review pass over the merged QUERY method surfaced a few remaining gaps:

- client: allow QUERY to follow redirects. client/core.go gated DoRedirects to
  GET/HEAD only, so QUERY silently ignored SetMaxRedirects. QUERY is
  safe+idempotent, so redirects are now followed with method and body preserved
  (verified with a 308 round-trip test).
- tests: assert QUERY appears in the OPTIONS and 405 Allow header; add QUERY
  cases to the earlydata and idempotency behavioral tests (both treat QUERY as
  safe via IsMethodSafe).
- docs: document the QUERY method (RFC 10008) in whats_new.md; it was absent
  from the v3 release notes.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
client/core.go (1)

116-127: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Eligibility logic is correct; consider deduplicating the repeated method check.

The QUERY addition is sound: per RFC 10008 QUERY is safe/idempotent, and transport.go only downshifts POSTGET on 301/302/303, so QUERY (like GET/HEAD) follows redirects preserving method and body. The identical three-method condition is now duplicated across the retry and non-retry branches; extracting a small helper would keep them from drifting.

♻️ Extract redirect-eligibility helper
func shouldFollowRedirects(method string, maxRedirects int) bool {
	return maxRedirects > 0 &&
		(method == fiber.MethodGet || method == fiber.MethodHead || method == fiber.MethodQuery)
}
-				if c.req.maxRedirects > 0 && (string(reqv.Header.Method()) == fiber.MethodGet || string(reqv.Header.Method()) == fiber.MethodHead || string(reqv.Header.Method()) == fiber.MethodQuery) {
+				if shouldFollowRedirects(string(reqv.Header.Method()), c.req.maxRedirects) {
 					return c.client.DoRedirects(reqv, respv, c.req.maxRedirects)
 				}
 				return c.client.Do(reqv, respv)
 			})
 		} else {
-			if c.req.maxRedirects > 0 && (string(reqv.Header.Method()) == fiber.MethodGet || string(reqv.Header.Method()) == fiber.MethodHead || string(reqv.Header.Method()) == fiber.MethodQuery) {
+			if shouldFollowRedirects(string(reqv.Header.Method()), c.req.maxRedirects) {
 				err = c.client.DoRedirects(reqv, respv, c.req.maxRedirects)
 			} else {
 				err = c.client.Do(reqv, respv)
 			}
 		}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/core.go` around lines 116 - 127, The redirect eligibility check in
core.go is duplicated in both the retry and non-retry branches, so extract it
into a small helper to prevent the method list from drifting. Add a helper such
as shouldFollowRedirects and use it from the logic around c.client.DoRedirects
and c.client.Do so the GET/HEAD/QUERY condition is defined once and reused
consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@client/core.go`:
- Around line 116-127: The redirect eligibility check in core.go is duplicated
in both the retry and non-retry branches, so extract it into a small helper to
prevent the method list from drifting. Add a helper such as
shouldFollowRedirects and use it from the logic around c.client.DoRedirects and
c.client.Do so the GET/HEAD/QUERY condition is defined once and reused
consistently.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: b298edcb-c7a9-40ca-a22e-d170a5136008

📥 Commits

Reviewing files that changed from the base of the PR and between e5fd66f and faee8dd.

📒 Files selected for processing (6)
  • app_test.go
  • client/core.go
  • client/request_test.go
  • docs/whats_new.md
  • middleware/earlydata/earlydata_test.go
  • middleware/idempotency/idempotency_test.go

@ReneWerner87
ReneWerner87 force-pushed the feat/http-query-method-followup branch from a819e6e to 948553f Compare June 25, 2026 10:03

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@middleware/cache/cache.go`:
- Around line 247-252: The QUERY body handling in the cache key path currently
materializes the fully escaped body string before `boundKeySegment` can decide
to hash it, causing unnecessary allocations for large bodies. Update the
`requestMethod == fiber.MethodQuery` branch to route body key generation through
a helper like `queryBodyKeySegment` that short-circuits long inputs and hashes
the escaped bytes incrementally instead of building the escaped string first;
keep the same escaping/bounding behavior for small bodies and preserve the
existing `escapeKeyDelimiters`, `boundKeySegment`, and `utils.UnsafeString`
semantics.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 675f2d12-a573-4fc9-807d-5974f25cddb5

📥 Commits

Reviewing files that changed from the base of the PR and between faee8dd and a819e6e.

📒 Files selected for processing (6)
  • docs/middleware/cache.md
  • docs/middleware/csrf.md
  • middleware/cache/cache.go
  • middleware/cache/cache_test.go
  • middleware/csrf/csrf.go
  • middleware/csrf/csrf_test.go
✅ Files skipped from review due to trivial changes (2)
  • docs/middleware/csrf.md
  • docs/middleware/cache.md

Comment thread middleware/cache/cache.go Outdated

@gaby gaby left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚡ LGTM

@ReneWerner87
ReneWerner87 merged commit aa08556 into main Jun 25, 2026
37 checks passed
@ReneWerner87
ReneWerner87 deleted the feat/http-query-method-followup branch June 25, 2026 12:43
@github-project-automation github-project-automation Bot moved this to Done in v3 Jun 25, 2026
@gaby gaby mentioned this pull request Jun 25, 2026
20 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants